Implemented BibtexkeyChecker #2022
Conversation
|
Could add tests and a CHANGELOG entry? Maybe in a follow up, could you address #1779 (comment) so that the jumping to an entry works better. @bartsch-dev Maybe you have an idea for a quick implementation of that? |
|
Tests: It is difficult to test for an unexisting variable in this test environment. I'll test only for "assertCorrect". |
| @@ -112,6 +112,12 @@ public void testMonthChecks() { | |||
| } | |||
|
|
|||
| @Test | |||
| public void testBibtexkeyChecks() { | |||
| assertCorrect(createContext("bibtexkey", "Knuth2014")); | |||
There was a problem hiding this comment.
Could you do one assertCorrect per Test please? 😇
There was a problem hiding this comment.
As we now have a class for each checker, we should also create a test class for each checker separately.
But I would think that is content for a new PR
There was a problem hiding this comment.
Removed my thumb up. A separate test class for every checker is too much (for evt. one or two entries). I think, it is better to organize the checker tests by fields (in methods) and not by checker classes.
|
Tests: I view the changes using the CodeCov plugin (see also https://github.com/JabRef/jabref/wiki/Tools) and this is the result:
I just wanted to ensure that everything is at least orange if not green 🎢 |
|
I think, it is difficult to implement empty bibtexkey in a clean way. The problem is that the bibtexkey is not existing, but the constructor IntegrityMessage requires an existing entry and field. So this might be the reason, why codecov is complaining. A solution could be to overload the constructor IntegrityMessage for this special case. |
|
|
||
| @Override | ||
| public List<IntegrityMessage> check(BibEntry entry) { | ||
| Optional<String> valuekey = entry.getField(BibEntry.KEY_FIELD); |
There was a problem hiding this comment.
Please use entry.getCiteKey.
| Optional<String> valueyear = entry.getField(FieldName.YEAR); | ||
| String authortitleyear = entry.getAuthorTitleYear(100); | ||
|
|
||
| if (!valueauthor.isPresent() || !valuetitle.isPresent() || !valueyear.isPresent()) { |
There was a problem hiding this comment.
I don't understand this if statement. So if an entry is missing an author (or title/year) and also has no bibtexkey, then no warning will be printed?
There was a problem hiding this comment.
This is because getAuthorTitleYear(i) requires, that all three field exist. Otherwise some check tests are failing.
There was a problem hiding this comment.
Judging based on the code, I think getAuthorTitleYear would return N/A: N/A (N/A) in the case where the fields are missing. I would suggest that you change the order of the if statements.
If the key is empty, then we definitely want to report a message. But if then all the author, title and year are empty, we use BibEntry.toString (it is a bit ugly) or otherwise we use getAuthorTitleYear.
There was a problem hiding this comment.
Changing the order of the if statements causes the fail of all checker tests:
java.lang.AssertionError: expected:<[]> but was:<[[] in bibtexkey: empty bibtexkey: N/A: "N/A" (N/A)]
| return Collections.emptyList(); | ||
| } | ||
|
|
||
| if (!valuekey.isPresent()) { |
There was a problem hiding this comment.
I would also check that the key is not empty (like in your second test below).
There is a similar convenient method in StringUtil already (I think it was called isNotBlank not sure if isBlank already exists but it should be easy to add).
* BibtexkeyChecker * CHANGELOG.md and tests * Changes 1 * checkstyle * Change 2 * Change 3
Check for empty bibtexkey. #1779.